Refactor of flow tools - OpenGraph.isclose
#374
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #374 +/- ##
==========================================
+ Coverage 86.25% 86.30% +0.05%
==========================================
Files 44 44
Lines 6161 6162 +1
==========================================
+ Hits 5314 5318 +4
+ Misses 847 844 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
matulni
changed the title
OpenGraph comparison.OpenGraph.isclose
graphix/opengraph.py
Outdated
| if og_1.input_nodes != og_2.input_nodes or og_1.output_nodes != og_2.output_nodes: | ||
| return False | ||
|
|
||
| return set(og_1.measurements.keys()) == set(og_2.measurements.keys()) |
There was a problem hiding this comment.
Isn't it redundant? If both og_1 and og_2 are well-formed, then these keys are their non-output nodes, and we already know that the two graphs have the same nodes and the same output nodes.
There was a problem hiding this comment.
Yes, in a context where we don't allow incorrect open graphs, this is redundant. However, I thought that we were moving away from this model, i.e., substituting the post_init by a check_well_formed method. Anticipating this design, I thought it might be interesting to ensure that two open graphs are equal, even if they are not correct (for debugging purposes, mainly). If you think the sacrifice in readability and efficiency is not worth, I'm ok with suppressing the check on the measured nodes.
There was a problem hiding this comment.
Okay, if the plan is to move towards allowing incorrect open graphs to be constructed and possibly checked afterwards, it makes sense to have a comparison function that even works on incorrect graphs.
There was a problem hiding this comment.
Hi again @thierry-martinez. When writing #381 and #378 I realised that it's quite practical to assume that open graphs are well formed. I propose that we keep (at least for now) the _post_init_ design pattern for open graphs.
I think that for open graphs this is justified because open graphs are more often input directly by the user (contrary to flow objects, which result from built-in generation routines).
In 5cd51d6, I removed the redundancy in the structural comparison of (well-formed) open graphs as you suggested.
graphix/opengraph.py
Outdated
| if not nx.utils.graphs_equal(og_1.graph, og_2.graph): | ||
| return False | ||
|
|
||
| if og_1.input_nodes != og_2.input_nodes or og_1.output_nodes != og_2.output_nodes: | ||
| return False |
There was a problem hiding this comment.
| if not nx.utils.graphs_equal(og_1.graph, og_2.graph): | |
| return False | |
| if og_1.input_nodes != og_2.input_nodes or og_1.output_nodes != og_2.output_nodes: | |
| return False | |
| return ( | |
| nx.utils.graphs_equal(og_1.graph, og_2.graph) | |
| and og_1.input_nodes == og_2.input_nodes | |
| and og_1.output_nodes == og_2.output_nodes | |
| ) |
|
Implemented minor modification from discussion in 9358992 |
This commit adapts the existing method `graphix.opengraph.OpenGraph.isclose` to the new API introduced in #358. Additionally, it introduces the new methods `graphix.opengraph.OpenGraph.is_equal_structurally` which compares the underlying structure of two open graphs, and `graphix.fundamentals.AbstractMeasurement.isclose` which defaults to `==` comparison.
3 participants
This commit adapts the existing method
:func: OpenGraph.iscloseto the new API introduced in #358.Additionally, it introduces a new method
:func: OpenGraph.__eq__relevant for open graphs of parametric typegraphix.fundamentals.Planeorgraphix.fundamentals.Axis.